Skip to content

Create AuthorizedKeysCommand#5236

Merged
techknowlogick merged 2 commits into
go-gitea:masterfrom
zeripath:AuthorizedKeysCommand
Nov 1, 2018
Merged

Create AuthorizedKeysCommand#5236
techknowlogick merged 2 commits into
go-gitea:masterfrom
zeripath:AuthorizedKeysCommand

Conversation

@zeripath
Copy link
Copy Markdown
Contributor

@zeripath zeripath commented Oct 31, 2018

This pull-request adds a subcommand that can be used as an SSH AuthorizedKeysCommand.

The configuration is described in command-line.md.

@zeripath zeripath force-pushed the AuthorizedKeysCommand branch from 0a044a7 to 6e94f2b Compare October 31, 2018 19:15
techknowlogick
techknowlogick reviewed Oct 31, 2018
View reviewed changes
Comment thread cmd/keys.go Outdated
@bkcsoft bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 31, 2018
@zeripath zeripath mentioned this pull request Oct 31, 2018
Merged
@zeripath zeripath force-pushed the AuthorizedKeysCommand branch from 6e94f2b to 7504f81 Compare October 31, 2018 19:40
@bkcsoft bkcsoft added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Oct 31, 2018
techknowlogick
techknowlogick approved these changes Oct 31, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@techknowlogick techknowlogick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit, Can you add setting to custom/conf/app.ini.sample

@zeripath
Copy link
Copy Markdown
Contributor Author

zeripath commented Oct 31, 2018 •
edited
Loading

@techknowlogick you mean add the NO_AUTHORIZED_KEYS_FILE=false setting to custom/conf/app.ini?

Hmm. I think if it's going to go in to the suggested app.ini by default, I should change it to SSH_CREATE_AUTHORIZED_KEYS_FILE=true so that we don't have a double negative.

@techknowlogick
Copy link
Copy Markdown
Member

techknowlogick commented Oct 31, 2018 via email •
edited
Loading

@zeripath zeripath force-pushed the AuthorizedKeysCommand branch 2 times, most recently from 8b8df05 to 9e87637 Compare October 31, 2018 20:28
@zeripath
Copy link
Copy Markdown
Contributor Author

zeripath commented Oct 31, 2018

Sorry I just noticed that XORM was reporting stuff on stdout that would conflict with the authorized_keys standard so adjusted the code.

@zeripath zeripath force-pushed the AuthorizedKeysCommand branch from 9b9f428 to 256508a Compare October 31, 2018 21:21
@zeripath zeripath force-pushed the AuthorizedKeysCommand branch from 256508a to 4b41181 Compare October 31, 2018 21:22
@codecov-io
Copy link
Copy Markdown

codecov-io commented Oct 31, 2018 •
edited
Loading

Codecov Report

Merging #5236 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5236      +/-   ##
==========================================
- Coverage   37.62%   37.61%   -0.01%     
==========================================
  Files         310      310              
  Lines       46039    46040       +1     
==========================================
- Hits        17320    17319       -1     
- Misses      26240    26242       +2     
  Partials     2479     2479
Impacted Files Coverage Δ
models/ssh_key.go 39.14% <100%> (ø) ⬆️
modules/setting/setting.go 48.31% <100%> (+0.06%) ⬆️
models/repo_list.go 63.29% <0%> (-1.27%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00533d3...0b5115f. Read the comment docs.

@techknowlogick techknowlogick added this to the 1.7.0 milestone Nov 1, 2018
@techknowlogick techknowlogick added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Nov 1, 2018
@bkcsoft bkcsoft added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Nov 1, 2018
@techknowlogick techknowlogick merged commit 7d9a191 into go-gitea:master Nov 1, 2018
@zeripath zeripath mentioned this pull request Nov 1, 2018
Closed
@zeripath
Copy link
Copy Markdown
Contributor Author

zeripath commented Nov 1, 2018

I guess this request partially closes #1870?

@zeripath zeripath deleted the AuthorizedKeysCommand branch November 1, 2018 22:50
@sapk
Copy link
Copy Markdown
Member

sapk commented Dec 14, 2019 •
edited
Loading

@zeripath I am looking at this to use it in the docker and remove the write to authorized_keys file but I can figure why the -type flag is required since not needed and seems not used. Do you have any insight ?

Don't mind I have understand. Sorry.

lafriks
lafriks reviewed Dec 15, 2019
View reviewed changes
Comment thread cmd/keys.go
return err
}

publicKey, err := models.SearchPublicKeyByContent(content)
Copy link
Copy Markdown
Member

@lafriks lafriks Dec 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use internal api not models directly

Copy link
Copy Markdown
Contributor Author

@zeripath zeripath Dec 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would have been a good spot >1 year ago... I guess we need to pop a bug fix up...

Copy link
Copy Markdown
Member

@lafriks lafriks Dec 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It just popped up in notifications 😅

@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@lafriks lafriks lafriks approved these changes

@techknowlogick techknowlogick techknowlogick approved these changes

Assignees

No one assigned

Labels

lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/feature Completely new functionality. Can only be merged if feature freeze is not active.

Projects

None yet

Milestone

1.7.0

Development

Successfully merging this pull request may close these issues.

6 participants

@zeripath @techknowlogick @codecov-io @sapk @lafriks @bkcsoft